Introduce a document id to support multiple documents in TiGLCreator#1401
Draft
joergbrech wants to merge 4 commits into
Draft
Introduce a document id to support multiple documents in TiGLCreator#1401joergbrech wants to merge 4 commits into
joergbrech wants to merge 4 commits into
Conversation
Give the shared TIGLCreatorContext scene a per-document shape manager and AIS-object tracking, keyed by a new DocumentId, instead of one global InteractiveShapeManager and a global deleteAllObjects(). This is prep for #1211 (multiple simultaneously open CPACS documents): today only one document is ever open, so a single fixed DocumentId is used everywhere and behavior is unchanged, but every display call and shape-manager access now goes through an explicit DocumentId rather than an implicit global, so a later PR can open several documents at once in the shared viewport without their shapes colliding by UID or getting wiped by each other's redraws/closes. Also adds TIGLCreatorContext::GetShapeFromIObject() for cross-document 3D-pick resolution (a click in the shared viewport can land on any open document's shape). Not yet covered by automated tests: TIGLCreator has no GUI test target, and this environment cannot run the GUI to manually verify runtime behavior. Verified via a clean full rebuild of every changed file (zero warnings/errors) and the existing 899 core-library unit tests (unaffected, src/ untouched). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AyogF1kmLgJiCG63GVzpNW
Contributor
PR RemindersThis PR has changes in
This is an automated reminder from CI |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1401 +/- ##
=======================================
Coverage 73.20% 73.20%
=======================================
Files 324 324
Lines 28598 28598
=======================================
Hits 20934 20934
Misses 7664 7664
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1399. Adresses #1211.
Today
TIGLCreatorContext(the shared OpenCASCADE 3D scene) has a single globalInteractiveShapeManagerand a single globaldeleteAllObjects(). That's fine as long as only one CPACS document is ever open, but it's the reason multiple simultaneously-open documents (#1211) can't work: two open documents could easily share a component UID (e.g. two sibling models from one CPACS file, or two variants of the same aircraft), which would silently collide in the shared shape manager, and closing/redrawing one document would wipe every document's shapes out of the viewport.This PR introduces a
DocumentId(TIGLCreator/src/DocumentId.h) and scopes the scene's shape bookkeepingto it:
GetShapeManager()→GetShapeManager(DocumentId), backed by astd::map<DocumentId, InteractiveShapeManager>instead of one shared instance.displayShape/displayPoint/displayVector/displayShapeHLMode/displayLineHLModecall now takes aDocumentIdand registers the resulting AIS object against it, so unnamed/untracked shapes (e.g. control-point-net previews) are also scoped correctly, not just named ones.deleteObjectsOfDocument(DocumentId)removes only one document's shapes from the scene, leaving other open documents' shapes untouched — the scoped replacement for the old globaldeleteAllObjects().GetShapeFromIObject(...)at the context level resolves a 3D-view pick across all open documents' shape managers, since a click in the shared viewport can land on any of them.I made the
DocumentIdparameter mandatory everywhere rather than defaulted, specifically so the compiler would force auditing every call site (there turned out to be ~150 of them acrossTIGLCreatorDocument.cpp, ModificatorModel.cpp,TIGLCreatorWidget.cpp,ModificatorDisplayOptionsWidget.cpp,TIGLCreatorWindow.cpp, andTIGLCreatorInputoutput.cpp`) rather than risk silently missing one.This PR is intentionally a pure backend refactor with no user-visible behavior change. Only one document is ever open today, so a single fixed
DocumentIdis used everywhere and every code path behaves exactly as before. It lays the groundwork for #1400 (tabbed CPACS tree for multiple open documents) and #1302 (auto-loading multiple models from one file), neither of which is possible without this.One subtlety worth calling out for review:
deleteObjectsOfDocument()intentionally does not clear a document's named shape-manager entries (unlike closing a document, which does, via a separateGetShapeManager(id).clear()call) — several redraw code paths rely onHasShapeEntry()still returning true for shapes that were removed from the viewport but not yet redrawn. Folding the clear into the delete would have subtly changed "draw only if not already displayed" behavior at ~14 call sites.How Has This Been Tested?
Not at all 😨
Screenshots, that help to understand the changes(if applicable):
Checklist for PR Author:
ChangeLog.mdupdated